-
-
Notifications
You must be signed in to change notification settings - Fork 20
GH-1196 Refactor sound sending system #1196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
GH-1196 Refactor sound sending system #1196
Conversation
…erQuitMessageController)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the sound sending system by moving the logic from dedicated listener classes into the relevant controllers, such as PlayerJoinMessageController
and ChatManagerController
. The package structure for session-related controllers has also been improved. The changes are logical, but I've identified a couple of areas for improvement. There's a potential logic issue in the chat sound handler that might prevent sounds from playing for users with bypass permissions. Additionally, the new sound playing logic is duplicated across three different controllers, and I've suggested creating a dedicated SoundService
to centralize this functionality and improve maintainability.
for (Player online : this.server.getOnlinePlayers()) { | ||
online.playSound(online.getLocation(), sound.afterJoin, sound.afterJoinVolume, sound.afterJoinPitch); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic for iterating over all online players to play a sound is duplicated in PlayerQuitMessageController.java
(lines 46-48) and ChatManagerController.java
(lines 115-117). To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, I suggest extracting this into a shared service, for example, a SoundService
.
This would centralize the sound playing logic, making it easier to manage and modify in the future. Here's an example of how such a service could look:
@Service
public class SoundService {
private final Server server;
@Inject
public SoundService(Server server) {
this.server = server;
}
public void playSoundToAll(Sound sound, float volume, float pitch) {
if (sound == null) {
return;
}
for (Player online : this.server.getOnlinePlayers()) {
online.playSound(online.getLocation(), sound, volume, pitch);
}
}
}
You could then inject SoundService
into the controllers and simplify the code to a single method call, like this.soundService.playSoundToAll(sound.afterJoin, sound.afterJoinVolume, sound.afterJoinPitch);
.
if (!this.chatSettings.chatEnabled()) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check if (!this.chatSettings.chatEnabled())
appears to be redundant and might cause unintended behavior. The event handler is already set to ignoreCancelled = true
, so it won't run if the chat event is cancelled (for example, when chat is disabled for a user without bypass permissions).
However, for a user with bypass permissions, their message will go through, but this check will prevent the sound from playing. It seems more consistent to play the sound if the message is successfully sent. I recommend removing this conditional block to fix this.
Moved sound sending functions e.g.
(PlayerQuitSoundController -> PlayerQuitMessageController)